-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support for specifying version numbers in API endpoints #1115
base: main
Are you sure you want to change the base?
Conversation
…d types in OpenAPI spec
I think this one is nearly ready to go (but after #1127). The one change I'm leaning towards is that I think |
@jgallagher and @leftwo raised the excellent point that earlier versions of this PR would cause a Dropshot server to happily serve requests for API versions newer than the server was built with. For now, we can make this the responsibility of the Stepping back: my goal for this work is to provide a minimal base for us to go prototype versioning in some real consumers. I think it's pretty likely that as we do that, we will find changes we want to make to this. I wouldn't be surprised if we wind up with some common impls of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice
} | ||
|
||
fn semver_parts(x: &semver::Version) -> (u64, u64, u64) { | ||
// This was validated during validation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems valid
let router = api.into_router(); | ||
if let VersionPolicy::Unversioned = version_policy { | ||
if router.has_versioned_routes() { | ||
return Err(BuildError::UnversionedServerHasVersionedRoutes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the point of has_versioned_routes
i.e. is it mostly here to produce an error message if the user has done something aberrant?
api.openapi("Evolving API", semver).write(&mut found).unwrap(); | ||
let actual = std::str::from_utf8(found.get_ref()).unwrap(); | ||
expectorate::assert_contents( | ||
&format!("tests/test_openapi_v{}.json", version), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about
&format!("tests/test_openapi_v{}.json", version), | |
&format!("tests/test_versions_v{}.json", version), |
or something to help associate json with rs?
"semver range (from ... until ...) has the \ | ||
endpoints out of order", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from version is after until version?
.into_iter() | ||
.collect(); | ||
return Err(syn::Error::new_spanned( | ||
span, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want
span, | |
input.span(), |
Maybe? In particular I'm thinking of this test:
dropshot/tests/fail/bad_version_backwards.stderr
error: semver range (from ... until ...) has the endpoints out of order
--> tests/fail/bad_version_backwards.rs:17:16
|
17 | versions = "1.2.3".."1.2.2"
|
Should we be underlining both?
v.value() | ||
.parse::<semver::Version>() | ||
.map_err(|e| { | ||
syn::Error::new_spanned(v, format!("expected semver: {}", e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do Semver::parse() errors look like? Are those more useful than something like "{} is not of the form MAJOR.MINOR.PATCH"?
Supersedes #862 (and is based on it). Fixes #869.
Also based on #1127. This includes a test that uses that option to make sure that operations have the same names in different versions, even when they have different handler function names.